Skip to content

Fix: resolution kwarg silently ignored in RFDETR.train()#933

Merged
Borda merged 11 commits intodevelopfrom
copilot/fix-image-size-resolution-argument
Apr 8, 2026
Merged

Fix: resolution kwarg silently ignored in RFDETR.train()#933
Borda merged 11 commits intodevelopfrom
copilot/fix-image-size-resolution-argument

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 8, 2026

TrainConfig extends plain BaseModel (extra fields ignored), so passing resolution=N to .train() was silently dropped — the model always trained at the default config resolution regardless of what the user set.

Changes

  • RFDETR.train() now absorbs resolution before forwarding remaining kwargs to get_train_config():
    • Validates divisibility by patch_size × num_windows (raises ValueError if not)
    • Applies directly to self.model_config.resolution
    • Conditionally updates positional_encoding_size only when the current config is formula-derived (PE == resolution // patch_size); pretrained-specific PE values are preserved
  • Docstring updated to document resolution as a first-class .train() kwarg

Usage

from rfdetr import RFDETRMedium

# RFDETRMedium: patch_size=16, num_windows=2 → resolution must be divisible by 32
model = RFDETRMedium()
model.train(dataset_dir="...", resolution=672)  # now correctly uses 672 instead of 576

Invalid resolutions raise immediately with a clear message:

ValueError: resolution=570 is not divisible by patch_size (16) × num_windows (2) = 32.
Choose a resolution that is a multiple of 32.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • https://storage.googleapis.com/rfdetr/nano_coco/checkpoint_best_regular.pth
    • Triggering command: /usr/bin/python python -m pytest tests/training/ src/ -x -v -m not gpu --ignore=tests/training/test_metrics_csv.py (http block)
    • Triggering command: /usr/bin/python python -m pytest src/rfdetr/detr.py::rfdetr.detr.RFDETR.optimize_for_inference -v (http block)
    • Triggering command: /usr/bin/python python -m pytest src/rfdetr/detr.py::rfdetr.detr.RFDETR.optimize_for_inference -v --tb=short (http block)
  • https://storage.googleapis.com/rfdetr/rf-detr-seg-n-ft.pth
    • Triggering command: /usr/bin/python python -m pytest tests/ src/ -v -m not gpu --ignore=tests/training/test_metrics_csv.py --ignore=src/rfdetr/detr.py --ignore=tests/try_instantiate_all_models.py (http block)
  • huggingface.co
    • Triggering command: /usr/bin/python python -m pytest tests/ src/ -v -m not gpu --ignore=tests/training/test_metrics_csv.py --ignore=src/rfdetr/detr.py --ignore=tests/try_instantiate_all_models.py (dns block)
  • images.cocodataset.org
    • Triggering command: /usr/bin/python python -m pytest tests/ src/ -v -m not gpu --ignore=tests/training/test_metrics_csv.py --ignore=src/rfdetr/detr.py --ignore=tests/try_instantiate_all_models.py (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI linked an issue Apr 8, 2026 that may be closed by this pull request
2 tasks
Copilot AI changed the title [WIP] Fix image size not changing with resolution argument Fix: resolution kwarg silently ignored in RFDETR.train() Apr 8, 2026
Copilot AI requested a review from Borda April 8, 2026 07:36
Copilot finished work on behalf of Borda April 8, 2026 07:36
@Borda Borda added the bug Something isn't working label Apr 8, 2026
@Borda Borda marked this pull request as ready for review April 8, 2026 19:42
Copilot AI review requested due to automatic review settings April 8, 2026 19:42
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 8, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79%. Comparing base (a6a080e) to head (873dc11).
⚠️ Report is 2 commits behind head on develop.

Additional details and impacted files
@@          Coverage Diff           @@
##           develop   #933   +/-   ##
======================================
  Coverage       79%    79%           
======================================
  Files           97     97           
  Lines         7846   7875   +29     
======================================
+ Hits          6195   6224   +29     
  Misses        1651   1651           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a bug where resolution=... passed to RFDETR.train() was silently ignored because it is a ModelConfig field (not a TrainConfig field), then adds tests to lock in the corrected behavior.

Changes:

  • Pop and handle resolution inside RFDETR.train() before calling get_train_config().
  • Validate resolution divisibility by patch_size × num_windows, and update model config fields accordingly.
  • Add unit tests for resolution override behavior and error cases.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File Description
src/rfdetr/detr.py Absorbs resolution in RFDETR.train(), validates it, and mutates model_config before building TrainConfig.
tests/training/test_detr_shim.py Adds tests ensuring the resolution kwarg is popped, applied to the model config, and invalid values raise.

Borda and others added 3 commits April 8, 2026 21:53
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- [resolve C1] use operator.index() to normalize resolution type; explicitly reject bool, non-positive, and non-integer values before divisibility check
- [resolve C2/OptionB] only update positional_encoding_size when the config currently derives it formulaically (PE == resolution // patch_size); RFDETRBase keeps its pretrained DINOv2 PE=37 unchanged to preserve checkpoint compatibility
- [resolve C4] reword docstring to describe Option B PE behaviour and persistent model_config mutation
- [resolve C5] replace Unicode × with ASCII * in error message for terminal/log-parser compatibility

---
Co-authored-by: Claude Code <noreply@anthropic.com>
- [resolve C6] keep Base PE test aligned with Option B (DINOv2 PE preserved)
- [resolve R3] add test_resolution_kwarg_updates_positional_encoding_size_for_formula_derived_config using RFDETRSmallConfig to verify PE IS updated for formula-derived configs
- [resolve R2] parametrize 7 invalid inputs (zero, negative, bools, floats, string) against new ValueError guard
- [resolve R4] replace hardcoded resolution literals with computed block_size * N so tests self-document config constraints

---
Co-authored-by: Claude Code <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Borda and others added 4 commits April 8, 2026 22:59
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…case literals

- Removed redundant type and value checks for resolution.
- Updated test cases to use a more distinct `valid_resolution` value (`block_size * 11`) for clarity.
@Borda Borda merged commit 5cc0de9 into develop Apr 8, 2026
25 checks passed
@Borda Borda deleted the copilot/fix-image-size-resolution-argument branch April 8, 2026 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Image size doesn't appear to change with resolution argument

3 participants